Revert "[MemProf] Add ambigous memprof attribute" #161717
Merged
+7
−46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit cf44f19.
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Teresa Johnson (teresajohnson) ChangesReverts llvm/llvm-project#157204 This caused issues in ThinLTO binaries because of the checking here, that didn't expect allocations needing cloning to have memprof metadata: llvm-project/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp Lines 5572 to 5582 in 9133fc8 I need to move the assert into the if check and guard by that condition. And add a more thorough test. Full diff: https://github.com/llvm/llvm-project/pull/161717.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Analysis/MemoryProfileInfo.h b/llvm/include/llvm/Analysis/MemoryProfileInfo.h
index be690a49767b4..571caf95f275d 100644
--- a/llvm/include/llvm/Analysis/MemoryProfileInfo.h
+++ b/llvm/include/llvm/Analysis/MemoryProfileInfo.h
@@ -59,14 +59,6 @@ LLVM_ABI std::string getAllocTypeAttributeString(AllocationType Type);
/// True if the AllocTypes bitmask contains just a single type.
LLVM_ABI bool hasSingleAllocType(uint8_t AllocTypes);
-/// Removes any existing "ambiguous" memprof attribute. Called before we apply a
-/// specific allocation type such as "cold", "notcold", or "hot".
-LLVM_ABI void removeAnyExistingAmbiguousAttribute(CallBase *CB);
-
-/// Adds an "ambiguous" memprof attribute to call with a matched allocation
-/// profile but that we haven't yet been able to disambiguate.
-LLVM_ABI void addAmbiguousAttribute(CallBase *CB);
-
/// Class to build a trie of call stack contexts for a particular profiled
/// allocation call, along with their associated allocation types.
/// The allocation will be at the root of the trie, which is then used to
diff --git a/llvm/lib/Analysis/MemoryProfileInfo.cpp b/llvm/lib/Analysis/MemoryProfileInfo.cpp
index 11602d29c1313..0c1f8dbd1119a 100644
--- a/llvm/lib/Analysis/MemoryProfileInfo.cpp
+++ b/llvm/lib/Analysis/MemoryProfileInfo.cpp
@@ -125,24 +125,6 @@ bool llvm::memprof::hasSingleAllocType(uint8_t AllocTypes) {
return NumAllocTypes == 1;
}
-void llvm::memprof::removeAnyExistingAmbiguousAttribute(CallBase *CB) {
- if (!CB->hasFnAttr("memprof"))
- return;
- assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous");
- CB->removeFnAttr("memprof");
-}
-
-void llvm::memprof::addAmbiguousAttribute(CallBase *CB) {
- // We may have an existing ambiguous attribute if we are reanalyzing
- // after inlining.
- if (CB->hasFnAttr("memprof")) {
- assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous");
- } else {
- auto A = llvm::Attribute::get(CB->getContext(), "memprof", "ambiguous");
- CB->addFnAttr(A);
- }
-}
-
void CallStackTrie::addCallStack(
AllocationType AllocType, ArrayRef<uint64_t> StackIds,
std::vector<ContextTotalSize> ContextSizeInfo) {
@@ -488,9 +470,6 @@ void CallStackTrie::addSingleAllocTypeAttribute(CallBase *CI, AllocationType AT,
StringRef Descriptor) {
auto AllocTypeString = getAllocTypeAttributeString(AT);
auto A = llvm::Attribute::get(CI->getContext(), "memprof", AllocTypeString);
- // After inlining we may be able to convert an existing ambiguous allocation
- // to an unambiguous one.
- removeAnyExistingAmbiguousAttribute(CI);
CI->addFnAttr(A);
if (MemProfReportHintedSizes) {
std::vector<ContextTotalSize> ContextSizeInfo;
@@ -550,7 +529,6 @@ bool CallStackTrie::buildAndAttachMIBMetadata(CallBase *CI) {
assert(MIBCallStack.size() == 1 &&
"Should only be left with Alloc's location in stack");
CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
- addAmbiguousAttribute(CI);
return true;
}
// If there exists corner case that CallStackTrie has one chain to leaf
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index c4f1b680a53ec..ddb95a4184756 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -3981,7 +3981,6 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::identifyClones(
void ModuleCallsiteContextGraph::updateAllocationCall(
CallInfo &Call, AllocationType AllocType) {
std::string AllocTypeString = getAllocTypeAttributeString(AllocType);
- removeAnyExistingAmbiguousAttribute(cast<CallBase>(Call.call()));
auto A = llvm::Attribute::get(Call.call()->getFunction()->getContext(),
"memprof", AllocTypeString);
cast<CallBase>(Call.call())->addFnAttr(A);
@@ -5643,7 +5642,6 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
// clone J-1 (J==0 is the original clone and does not have a VMaps
// entry).
CBClone = cast<CallBase>((*VMaps[J - 1])[CB]);
- removeAnyExistingAmbiguousAttribute(CBClone);
CBClone->addFnAttr(A);
ORE.emit(OptimizationRemark(DEBUG_TYPE, "MemprofAttribute", CBClone)
<< ore::NV("AllocationCall", CBClone) << " in clone "
diff --git a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
index d1c0f643f5cd7..d8457a30fd2f7 100644
--- a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
+++ b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
@@ -230,8 +230,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -280,8 +279,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -335,8 +333,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -395,8 +392,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -467,8 +463,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
ASSERT_NE(Call, nullptr);
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals));
@@ -541,8 +536,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
// Restore original option value.
MemProfKeepAllNotColdContexts = OrigMemProfKeepAllNotColdContexts;
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals));
@@ -670,8 +664,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
// The hot allocations will be converted to NotCold and pruned as they
// are unnecessary to determine how to clone the cold allocation.
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
|
|
@llvm/pr-subscribers-llvm-transforms Author: Teresa Johnson (teresajohnson) ChangesReverts llvm/llvm-project#157204 This caused issues in ThinLTO binaries because of the checking here, that didn't expect allocations needing cloning to have memprof metadata: llvm-project/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp Lines 5572 to 5582 in 9133fc8 I need to move the assert into the if check and guard by that condition. And add a more thorough test. Full diff: https://github.com/llvm/llvm-project/pull/161717.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Analysis/MemoryProfileInfo.h b/llvm/include/llvm/Analysis/MemoryProfileInfo.h
index be690a49767b4..571caf95f275d 100644
--- a/llvm/include/llvm/Analysis/MemoryProfileInfo.h
+++ b/llvm/include/llvm/Analysis/MemoryProfileInfo.h
@@ -59,14 +59,6 @@ LLVM_ABI std::string getAllocTypeAttributeString(AllocationType Type);
/// True if the AllocTypes bitmask contains just a single type.
LLVM_ABI bool hasSingleAllocType(uint8_t AllocTypes);
-/// Removes any existing "ambiguous" memprof attribute. Called before we apply a
-/// specific allocation type such as "cold", "notcold", or "hot".
-LLVM_ABI void removeAnyExistingAmbiguousAttribute(CallBase *CB);
-
-/// Adds an "ambiguous" memprof attribute to call with a matched allocation
-/// profile but that we haven't yet been able to disambiguate.
-LLVM_ABI void addAmbiguousAttribute(CallBase *CB);
-
/// Class to build a trie of call stack contexts for a particular profiled
/// allocation call, along with their associated allocation types.
/// The allocation will be at the root of the trie, which is then used to
diff --git a/llvm/lib/Analysis/MemoryProfileInfo.cpp b/llvm/lib/Analysis/MemoryProfileInfo.cpp
index 11602d29c1313..0c1f8dbd1119a 100644
--- a/llvm/lib/Analysis/MemoryProfileInfo.cpp
+++ b/llvm/lib/Analysis/MemoryProfileInfo.cpp
@@ -125,24 +125,6 @@ bool llvm::memprof::hasSingleAllocType(uint8_t AllocTypes) {
return NumAllocTypes == 1;
}
-void llvm::memprof::removeAnyExistingAmbiguousAttribute(CallBase *CB) {
- if (!CB->hasFnAttr("memprof"))
- return;
- assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous");
- CB->removeFnAttr("memprof");
-}
-
-void llvm::memprof::addAmbiguousAttribute(CallBase *CB) {
- // We may have an existing ambiguous attribute if we are reanalyzing
- // after inlining.
- if (CB->hasFnAttr("memprof")) {
- assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous");
- } else {
- auto A = llvm::Attribute::get(CB->getContext(), "memprof", "ambiguous");
- CB->addFnAttr(A);
- }
-}
-
void CallStackTrie::addCallStack(
AllocationType AllocType, ArrayRef<uint64_t> StackIds,
std::vector<ContextTotalSize> ContextSizeInfo) {
@@ -488,9 +470,6 @@ void CallStackTrie::addSingleAllocTypeAttribute(CallBase *CI, AllocationType AT,
StringRef Descriptor) {
auto AllocTypeString = getAllocTypeAttributeString(AT);
auto A = llvm::Attribute::get(CI->getContext(), "memprof", AllocTypeString);
- // After inlining we may be able to convert an existing ambiguous allocation
- // to an unambiguous one.
- removeAnyExistingAmbiguousAttribute(CI);
CI->addFnAttr(A);
if (MemProfReportHintedSizes) {
std::vector<ContextTotalSize> ContextSizeInfo;
@@ -550,7 +529,6 @@ bool CallStackTrie::buildAndAttachMIBMetadata(CallBase *CI) {
assert(MIBCallStack.size() == 1 &&
"Should only be left with Alloc's location in stack");
CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
- addAmbiguousAttribute(CI);
return true;
}
// If there exists corner case that CallStackTrie has one chain to leaf
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index c4f1b680a53ec..ddb95a4184756 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -3981,7 +3981,6 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::identifyClones(
void ModuleCallsiteContextGraph::updateAllocationCall(
CallInfo &Call, AllocationType AllocType) {
std::string AllocTypeString = getAllocTypeAttributeString(AllocType);
- removeAnyExistingAmbiguousAttribute(cast<CallBase>(Call.call()));
auto A = llvm::Attribute::get(Call.call()->getFunction()->getContext(),
"memprof", AllocTypeString);
cast<CallBase>(Call.call())->addFnAttr(A);
@@ -5643,7 +5642,6 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
// clone J-1 (J==0 is the original clone and does not have a VMaps
// entry).
CBClone = cast<CallBase>((*VMaps[J - 1])[CB]);
- removeAnyExistingAmbiguousAttribute(CBClone);
CBClone->addFnAttr(A);
ORE.emit(OptimizationRemark(DEBUG_TYPE, "MemprofAttribute", CBClone)
<< ore::NV("AllocationCall", CBClone) << " in clone "
diff --git a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
index d1c0f643f5cd7..d8457a30fd2f7 100644
--- a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
+++ b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
@@ -230,8 +230,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -280,8 +279,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -335,8 +333,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -395,8 +392,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
CallBase *Call = findCall(*Func, "call");
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -467,8 +463,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
ASSERT_NE(Call, nullptr);
Trie.buildAndAttachMIBMetadata(Call);
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals));
@@ -541,8 +536,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
// Restore original option value.
MemProfKeepAllNotColdContexts = OrigMemProfKeepAllNotColdContexts;
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals));
@@ -670,8 +664,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
// The hot allocations will be converted to NotCold and pruned as they
// are unnecessary to determine how to clone the cold allocation.
- EXPECT_TRUE(Call->hasFnAttr("memprof"));
- EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
+ EXPECT_FALSE(Call->hasFnAttr("memprof"));
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
|
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
Reverts llvm#157204 This caused issues in ThinLTO binaries because of the checking here, that didn't expect allocations needing cloning to have memprof metadata: https://github.com/llvm/llvm-project/blob/9133fc8cb04f8e45c9b46de85a8de99bf01e55c7/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp#L5572-L5582 I need to move the assert into the if check and guard by that condition. And add a more thorough test.
teresajohnson
added a commit
to teresajohnson/llvm-project
that referenced
this pull request
Oct 3, 2025
Reapply llvm#157204 with fix and a new test for the issue it caused (the test change provoked the assert that was converted to an if condition). Also, make the application of this new attribute under an (on by default) flag, so that it can be more easily disabled if needed. Add test for the new flag.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #157204
This caused issues in ThinLTO binaries because of the checking here, that didn't expect allocations needing cloning to have memprof metadata:
llvm-project/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
Lines 5572 to 5582 in 9133fc8
I need to move the assert into the if check and guard by that condition. And add a more thorough test.